-
-
Notifications
You must be signed in to change notification settings - Fork 7.4k
fix(build): ensure amd bundles request require
to be injected
#20861
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…module instead of the baseURI
951ec02
to
835e172
Compare
Would you provide concrete examples of |
Sure, no problem. I've used this code: const customRelativeUrlMechanisms = {
...relativeUrlMechanisms,
// override amd to use module.uri instead of document.baseURI
amd: (relativePath) => {
if (relativePath[0] !== '.') relativePath = './' + relativePath
return getResolveUrl(
`require.toUrl('${escapeId(relativePath)}'), (() => {
console.log('relativePath', '${relativePath}')
console.log('require.toUrl(relativePath)', require.toUrl('${relativePath}'))
console.log('module.id', module.id)
console.log('module.uri', module.uri)
console.log('document.baseURI', document.baseURI)
return new URL(module.uri, document.baseURI).href
})()`,
)
},
'worker-iife': (relativePath) =>
getResolveUrl(
`'${escapeId(partialEncodeURIPath(relativePath))}', self.location.href`,
),
} as const satisfies Record<string, (relativePath: string) => string> to produce this output:
|
I'm not sure about the baseUrl - is it possible to have multiple instances of requirejs?
that's only valid for a single app but not for all others. No idea why rollup implemented it this way. Maybe |
If you insist, I can also send a PR to Rollup and we can check their feedback 😅 |
Thank you for the examples and the PR to Rollup. I think it makes sense. Just in case, I'd like to wait for a week for the feedback on Rollup side. |
Makes sense. I understand that while being a small change in terms of LoC, the change could potentlally be delicate and I certainly don't want to break others. If rollup people see a problem, I'm happy to discuss alternatives with you and them. |
I've tried to upstream it to rollup, but afaict now it's not applicable there, because rollup does not have the relative base feature https://vite.dev/guide/build.html#relative-base: relative paths are always relative to the baseURI not to the including module. In vite on the other hand the code I modified/overrode is only used when the relative base feature (with base c.f. rollup/rollup#6129 (comment) @sapphi-red Thoughts? |
I think the vite/packages/vite/src/node/build.ts Line 1407 in 6ad5424
So if there's a problem with Rollup, I guess we have the same problem with Vite. |
I think I need a reproduction to see what is happening. |
In my tests it wasn't the same value but I'll try to setup a reproducer which does (not) work with vite/rollup It's not completely trivial, so I'm not sure how soon I'll get to it... |
https://github.com/dschmidt/vite-amd-relative-import-example This shows my issue in vite for starters... |
I found the difference. With Rollup, the following code is generated: define(["require"], (function(require) {
"use strict";
const fooTxtUrl = new URL(require.toUrl("../assets/foo.txt"), document.baseURI).href;
function main() {
console.log("fooTxtUrl", fooTxtUrl);
return "OK";
}
return main;
})); On the other hand, Vite generates: define((function() {
"use strict";
const fooTxtUrl = "" + new URL(require.toUrl("../assets/foo.txt"), document.baseURI).href;
function main() {
console.log("fooTxtUrl", fooTxtUrl);
return "OK";
}
return main;
})); The problem here is that Vite is not generating |
That makes a lot of sense! I can confirm that adjusting the generated |
... to avoid falling back to the global require, which can break relative import urls.
require
to be injected
Description
make relative imports in amd/cjs relative to the current module instead of the baseURI.
Fixes #20860
I've added an override tocustomRelativeUrlMechanisms
to avoid modifying the copied code from rollup.As far as I can tell rollup itself usesmodule.uri
to replaceimport.meta.url
inamd
modules. So I assume it's okay to use it.https://github.com/rollup/rollup/blob/ce6cb93098850a46fa242e37b74a919e99a5de28/src/ast/nodes/MetaProperty.ts#L209An alternative could be to useresolve.toUrl(module.id)
but it seems unneccessary if other parts of the code already rely onmodule.uri
.FWIW I've verified thatimport.meta.url
gets transpiled tonew URL(module.uri, document.baseURI).href
in my setup with amd modules. Usingimport.meta.url
directly for the imports here does not work as it happens too late in the transpilation process (getting errors thatmodule
may not be used outside of module scope).I'm a bit lost regarding a regression test as I couldn't find a test for the relative path behavior that I could simply adjust... if you require me to implement one, could you give me a few pointers how to implement this specific one?After creating a proper reproducer and discussions with @lukastaegert of rollup fame and @sapphi-red it turned out, joining the path with the baseURI is actually correct.
It works in rollup because modules request
require
to be injected. This localrequire
instance has the correct base path and sotoUrl
returns a correct absolute path.In vite this was missing, so we were falling back to the global
require
, sotoUrl
used baseUri as base instead of the current module's path.This PR adds
require
to the dependencies of amd modules.